Fix race conditions due to non-volatile lazy vals #2506
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These races could be observed by running: dotty-compiler-bootstrapped/test-only *.CompilationTests These never happened on the CI so far because the order in which the classes are run is not deterministic and we got "lucky" that the first tests running the compiler did not use parallelism. The races were caused by a combination of multiple factors: - The new classpath implementation imported from 2.12 has a shared global cache of ClassPath instances (see ZipAndJarFileLookupFactory.scala) - If multiple threads try to initialize a lazy val in dotty, only one will succeed and the other will get null (in scalac, each thread will redo the initialization) - The classpath implementation relies on code from scala.reflect.io that was recently imported into dotty, this code contains fields with lazy vals. The race with the lazy val in ZipArchive was easy to fix since it immediately lead to NullPointerException, but the one in AbstractFile was much harder to find because the only method called on "lazy val extension" is `==` which works with null. This suggests that we should implement a compiler option to check for lazy val races at runtime. In both cases we fixed the issue by simply dropping the `lazy` instead of adding an `@volatile`, for ZipArchive I cannot think of a usecase where you would not want to force the lazy val, and for `AbstractFile` the cost of always computing the extension is probably less than adding a lock.
|
See also #2507 |
|
Great detective work! |
|
LGTM |
smarter
referenced
this pull request
in retronym/scala
Jun 9, 2017
Also make the entries of the cache weak references.
|
https://liufengyun.github.io/bench/ The benchmarks show that this PR causes a 2x slow down for one test case(more tests are in process). |
nicolasstucki
added a commit
to dotty-staging/dotty
that referenced
this pull request
Jul 26, 2017
As stated in scala#2506 `lazy` was droped to make it volatile. It turns out that the dirs is not always used and epensive to compute. In `implicit_cache.scala` only need dirs 1 out of 30 times.
liufengyun
added a commit
that referenced
this pull request
Jul 26, 2017
Alternative fix for #2506 that does not force dirs
|
@smarter What do you mean by "(in scalac, each thread will redo the initialization" ? Lazy vals initializers that terminate only execute once. |
|
@retronym You're right, I got confused with recursive lazy vals, the difference is that in scalac lazy vals in fields are synchronized by default. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These races could be observed by running:
dotty-compiler-bootstrapped/test-only *.CompilationTests
These never happened on the CI so far because the order in which the
classes are run is not deterministic and we got "lucky" that the first
tests running the compiler did not use parallelism.
The races were caused by a combination of multiple factors:
global cache of ClassPath instances (see ZipAndJarFileLookupFactory.scala)
will succeed and the other will get null (in scalac, each thread will
redo the initialization)
was recently imported into dotty, this code contains fields with lazy vals.
The race with the lazy val in ZipArchive was easy to fix since it
immediately lead to NullPointerException, but the one in AbstractFile
was much harder to find because the only method called on "lazy val
extension" is
==which works with null. This suggests that we shouldimplement a compiler option to check for lazy val races at runtime.
In both cases we fixed the issue by simply dropping the
lazyinsteadof adding an
@volatile, for ZipArchive I cannot think of a usecasewhere you would not want to force the lazy val, and for
AbstractFilethe cost of always computing the extension is probably less than adding
a lock.